Skip to content

Conversation

@buffalojoec
Copy link

Problem

The Config program has been migrated to an on-chain BPF program. It's time to remove it from the monorepo.

Summary of Changes

Commit-by-commit, unwind the Config program's API being used around the validator, then finally drop the program entirely.

@mergify
Copy link

mergify bot commented Mar 17, 2025

The Firedancer team maintains a line-for-line reimplementation of the
native programs, and until native programs are moved to BPF, those
implementations must exactly match their Agave counterparts.
If this PR represents a change to a native program implementation (not
tests), please include a reviewer from the Firedancer team. And please
keep refactors to a minimum.

@buffalojoec buffalojoec changed the title Drop the Config program Drop the Config builtin Mar 17, 2025
@buffalojoec

This comment was marked as resolved.

@buffalojoec buffalojoec force-pushed the drop-config branch 5 times, most recently from 979478b to bb88b19 Compare March 19, 2025 13:33
@buffalojoec buffalojoec marked this pull request as ready for review March 19, 2025 13:41
@buffalojoec
Copy link
Author

@tao-stones I've requested your review here, since the eviction of the Config program modifies the builtin cost tracker setup.

The contributor warnings around the builtin tracker list explicitly mention how to treat existing builtins that are going to be migrated and those that are not going to be migrated. However, it does not say anything about builtins that have been migrated on all three clusters.

Based on our previous conversations in SIMD 0170 and the codebase within solana-builtins-default-costs, removing a builtin from this list that is already 100% migrated is appropriate.

Let me know if you feel otherwise!

@buffalojoec
Copy link
Author

buffalojoec commented Mar 19, 2025

@topointon-jump FYI this PR touches the stake program, but it does not practically change anything about it. It merely updates it to use the Config program's client, rather than the builtin library from this repository. Same interface.

@buffalojoec buffalojoec requested a review from a team as a code owner March 19, 2025 14:11
Copy link

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look good from builtin cost perspective. config will no longer be builtin, its default cost will be 200K CUs.

@apfitzge
Copy link

Looks correct to me, but have a general question about builtin migrations that is related.

I see that we use core_bpf_programs() to add the bpf accounts for our program-test and test-validator...how does this work for a new cluster? are the accounts added in genesis or something?

@buffalojoec
Copy link
Author

Looks correct to me, but have a general question about builtin migrations that is related.

I see that we use core_bpf_programs() to add the bpf accounts for our program-test and test-validator...how does this work for a new cluster? are the accounts added in genesis or something?

Yes, exactly! Here:

agave/scripts/run.sh

Lines 74 to 77 in f56845e

./fetch-core-bpf.sh
if [[ -r core-bpf-genesis-args.sh ]]; then
CORE_BPF_GENESIS_ARGS=$(cat core-bpf-genesis-args.sh)
fi

https://github.com/anza-xyz/agave/blob/master/fetch-core-bpf.sh

Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me! Just a couple of small questions

@buffalojoec

This comment was marked as resolved.

@buffalojoec buffalojoec marked this pull request as draft April 2, 2025 10:53
@buffalojoec buffalojoec marked this pull request as ready for review April 2, 2025 14:53
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 94.23077% with 3 lines in your changes missing coverage. Please review.

Project coverage is 83.3%. Comparing base (0a0cce2) to head (223260d).
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #5331     +/-   ##
=========================================
- Coverage    83.3%    83.3%   -0.1%     
=========================================
  Files         828      824      -4     
  Lines      373649   372824    -825     
=========================================
- Hits       311592   310761    -831     
- Misses      62057    62063      +6     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@buffalojoec buffalojoec requested a review from tao-stones April 3, 2025 02:54
@buffalojoec
Copy link
Author

The node has hummed over the epoch rollover and everything seems good. CI passing, comments resolved. Ready to merge.

@tao-stones I re-requested you for the Fees codeowner. Thanks!

Copy link

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vouch for fee

@buffalojoec buffalojoec merged commit c7d7987 into anza-xyz:master Apr 4, 2025
59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants